ADFA-5530: the metrics carousel, consolidated onto stage - #1812
ADFA-5530: the metrics carousel, consolidated onto stage#1812davidschachterADFA wants to merge 234 commits into
Conversation
Enroll SwipeRevealLayout.kt in the file-level Spotless ratchet ahead of the ADFA-5487 functional change, so the whole-file reindent to tabs is not reviewer noise in a behavioral commit. ktlint changes only: import ordering, parameter list wrapping, and `return x` to expression-body conversions. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
RightDragCallback.tryCaptureView returned an unconditional `true`, with the intended check commented out as `// child.id == R.id.right_drawer_sidebar` -- an id that exists nowhere in the project. There is no right drawer in activity_editor.xml, so the helper had no legitimate target but captured whichever child sat under a horizontal drag and offset it sideways. Two consequences, both fixed by never capturing: - onViewPositionChanged pushed that horizontal travel straight to dragListener.onDragProgress, bypassing the layout's own onDragProgress. BaseEditorActivity.onSwipeRevealDragProgress then animated the content card's corner interpolation and top padding as if the vertical reveal were being dragged. - onInterceptTouchEvent returns `isLeft || isRight || isVertical`, so the layout stole horizontal gestures from its children. A horizontally scrolling child raced this helper across the same ViewConfiguration touch slop, making the outcome nondeterministic. ADFA-5487 puts a ViewPager2 carousel in exactly that position, which is how this surfaced. No edge tracking is configured, so with capture refused the helper is inert. The callback is left in place as the attachment point for a right drawer, should one ever be added. Verified: :app:compileV8DebugKotlin. ADFA-5487 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
BaseEditorActivity drove the memory chart by reaching into binding.memUsageView.chart from six sites and mutating entry.y against a pidToDatasetIdxMap that only resetMemUsageChart() populated. That works only while exactly one chart view exists for the activity's lifetime. ADFA-5487 makes the chart one page of a carousel, where the view can be unbound, recycled, or created long after watching began. MemoryUsageChartRenderer owns the chart wiring instead and holds no sample state: MemoryUsageWatcher already keeps each process's usageHistory ring buffer, so the renderer can rebuild a complete chart from getMemoryUsages() at any time. attach/detach are independent of the data. Two behaviour changes, both deliberate: - attach() renders the full existing history. resetMemUsageChart() used to seed every entry with 0f and wait a tick for real values, which a carousel page bound mid-session would show as a flat line. - onUsagesChanged() rebuilds when the incoming processes no longer match the chart's datasets, instead of logging "No dataset found for process" and dropping that process's samples. This was already reachable without a carousel: ProjectHandlerActivity watches the Gradle Tooling process and then calls resetMemUsageChart(), so any sample arriving between those two lines was discarded. The once-a-second path still mutates the existing Entry objects in place and allocates nothing; the rebuild is the exception, not the rule. The renderer relies on ChartData.getDataSetByIndex returning null for an out-of-range index, which the shipped AndroidChart 3.1.0.21 bytecode confirms (null for index < 0 or >= size) -- the same guard the previous code depended on. Sites swept: all six chart call sites in BaseEditorActivity, both resetMemUsageChart() callers in ProjectHandlerActivity (unchanged, the method keeps its signature), and the now-dead pidToDatasetIdxMap/editorSurfaceContainerBackground members and their imports. No other module referenced either. Tests: 5 new Robolectric tests in MemoryUsageChartRendererTest. Verified they fail without the fix -- reverting the two behaviour changes fails "attach renders the complete existing history", "attach after detach renders the history into the new chart" (all-zero entries) and "onUsagesChanged rebuilds when a process starts being watched" (dataSetCount stays 1), each for the reason it is named for. The in-place-update test passes either way by design, since that path is unchanged. Verified: :app:compileV8DebugKotlin, :app:testV8DebugUnitTest (MemoryUsageChartRendererTest, 5/5). No UI change, so no font-scale check yet; that lands with the carousel. ADFA-5487 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
The chart at the top of the editor (revealed by dragging the app bar
down) is now a ViewPager2 carousel. Page 1 is the memory chart, still
the default; page 2 is the Code On The Go brand mark, a placeholder
until there is a real second metric.
MetricsCarouselAdapter takes its page list as a constructor argument, so
the follow-up tickets (a TrafficStats network chart, and plugin-
contributed displays) add pages rather than change this class. The chart
page attaches MemoryUsageChartRenderer on bind and detaches on recycle;
because the renderer rebuilds from MemoryUsageWatcher's history, swiping
away and back shows the full 30-sample series rather than a flat line.
Layout notes:
- layout_mem_usage.xml stays a single view. SwipeRevealLayout asserts
childCount == 2 and indexes its children positionally, so the include
cannot gain a sibling; the pager and indicator live inside it.
- The status-bar inset now applies to the pager rather than the chart,
so MemoryUsageChartRenderer.setTopMargin (a shim from the previous
commit, when the activity owned the only chart) is gone. It gains
detachIfAttached, which a recycling container needs: RecyclerView can
bind a replacement view before recycling the one it replaced, and an
unconditional detach would then drop the new chart.
- editor_mem_usage_view_height goes 200dp -> 248dp. The indicator is new
chrome, so the container grows by its 48dp rather than the chart
shrinking. This is a visible change beyond the ticket's literal scope;
it is here because of the touch-target point below.
- TabLayout has no dot mode, so each tab's background is a selector and
the sliding indicator is suppressed. The oval needs a sized, centred
layer-list item: a tab background is stretched to fill the tab, which
ignores a bare shape's <size> and renders an oval as tall as the whole
row. The active dot differs in both size and colour because several of
this app's themes resolve colorPrimary to a grey indistinguishable
from colorOutline (measured on device: #AAAAAA vs #8F9099).
A left-to-right swipe cannot page backwards: that gesture opens the
navigation drawer, which is documented app behaviour ("To view the file
tree and project options, swipe from left to right", shown in the
editor's own onboarding text). InterceptableDrawerLayout's
findScrollingChild starts at index 1 and so never examines DrawerLayout's
content child, which is consistent with that intent. Backward navigation
is therefore by tapping the indicator, which makes the dots a primary
control rather than decoration -- hence real 48dp touch targets,
measured on device at 48x48dp (168x168px at 560dpi), each carrying a
"Metric N of 2" content description.
androidx.viewpager2 is declared explicitly. It was already on the
compile classpath transitively and pinned to the same 1.1.0-beta02 the
version catalog names, so this adds no new dependency; it just stops a
compile-time use depending on another library's graph.
Verified on a Pixel 6 Pro (arm64), v8 debug:
- Both pages render; swipe forward and tap-to-navigate both directions.
- Returning to page 1 shows the complete history for both watched
processes, including a Gradle Tooling process that started while the
carousel was open (the rebuild path from the previous commit).
- Font scale 1.0 and 2.0: no clipping, no overlap, status bar clear,
touch targets unchanged. MPAndroidChart sizes its own text in pixels
so the chart labels do not grow with font scale -- pre-existing, and
worth a follow-up for low-vision users.
- Landscape: renders correctly, nothing clipped.
- :app:testV8DebugUnitTest for ui/activities/fragments: 41 tests green.
ADFA-5487
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
The carousel could only page forwards. A left-to-right swipe opened the navigation drawer instead, so going back needed a tap on the indicator dots, which in turn forced them to be 48dp touch targets. Two mechanisms claim that gesture, and each needs its own answer: - View-hierarchy interceptors. MetricsCarouselLayout, the new root of layout_mem_usage.xml, calls requestDisallowInterceptTouchEvent on its ancestors on ACTION_DOWN. That propagates the whole way up, so any ancestor ViewGroup is out of the way for the rest of the gesture, and only for gestures starting inside this strip. - The editor's activity-level GestureDetector, run from dispatchTouchEvent. It never calls onInterceptTouchEvent, so no disallow-intercept can stop it; this was in fact the one opening the drawer, confirmed on device. isTouchOnMetricsCarousel excludes the carousel's bounds the same way isTouchOnBottomSheetTabs already excludes the bottom-sheet tab strip. The exclusion is gated on swipeReveal.dragProgress > 0. The carousel is laid out at the top of the reveal even while the content card covers it, and siblings do not clip each other, so getGlobalVisibleRect reports it visible either way; without the gate the drawer gesture would have gone dead over the top of a closed editor. The vertical reveal drag is unaffected: SwipeRevealLayout only captures a vertical drag whose touch-down landed in its drag handle (the app bar), never in this strip. With swipe working both ways the dots are a status indicator rather than a control, so they no longer need 48dp targets or accessibility nodes of their own -- ViewPager2 already reports page position, and each page carries its own content description. Touches on the indicator are swallowed so the dots cannot act as tabs, while TabLayoutMediator still tracks the selected page. The row drops 48dp -> 20dp and, with the panel kept at 248dp, that space goes to the chart: the plot area grows from 135dp to 187dp. The now-unused metrics_carousel_page string is removed. Verified on a Pixel 6 Pro (arm64), v8 debug: - Paging forward and backward by swipe, portrait and landscape. - Returning to page 1 still shows full history for both watched processes. - Drawer gesture unaffected: still opens from a rightward fling outside the carousel while the reveal is open, and from one over the region the carousel occupies once the reveal is closed. - Font scale 1.0 and 2.0: geometry is dp-only and unchanged (pager and indicator bounds identical at both), nothing clipped, status bar clear. - :app:testV8DebugUnitTest for ui/activities/fragments: 41 tests green. ADFA-5487 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
Dots said which page you were on but not what it was. A metrics
carousel is a set of different displays, so naming the current one
carries more information in the same space: "Memory usage" rather than
two dots.
MetricsPage gains a title, so a page names itself and the follow-up
tickets (network chart, plugin-contributed pages) supply one as a
matter of course. A ViewPager2.OnPageChangeCallback drives the label;
it is unregistered alongside the adapter in preDestroy. The callback
does not fire for the page the carousel opens on, so the initial title
is set explicitly.
The title is sp text, unlike the dp-sized dots, so the layout had to
change shape: the title is wrap_content and the pager takes whatever
height is left. At 2x font scale the title grows from 22dp to 35dp and
the chart gives up that space, rather than the label clipping or the
panel changing height. No maxLines or ellipsize -- a long title wraps
and the chart absorbs it, which is the right failure mode for text that
is not disposable.
This drops the TabLayout, the dot selector drawable, its four dimens,
and the touch-swallowing needed to stop dots acting as tabs. The dots'
theme problem goes with them: the active dot needed to differ in both
size and colour because several themes resolve colorPrimary to a grey
indistinguishable from colorOutline.
Trade-off: a title does not show that further pages exist, which dots
did. Worth revisiting if the carousel grows past a handful of pages; at
two, swiping finds the second one and the title then says what it is.
Verified on a Pixel 6 Pro (arm64), v8 debug:
- Titles track the page ("Memory usage", "Code On The Go"); paging both
directions still works and page 1 still returns with full history.
- Font scale 1.0 and 2.0, measured on a cold start: title 22dp -> 35dp,
pager 185dp -> 171dp, panel 248dp throughout, nothing clipped.
EditorActivityKt declares fontScale in configChanges, so it is not
recreated on a font-scale change -- a warm relaunch reports stale
geometry and the app must be force-stopped first to measure this.
- :app:testV8DebugUnitTest for ui/activities/fragments: 41 tests green.
ADFA-5487
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
Second page of the editor's metrics carousel (ADFA-5487) is now a live network traffic chart, replacing the brand-mark placeholder. Accounting is UID-level, as decided on the ticket: TrafficStats.getUidRxBytes / getUidTxBytes cover every process sharing the app's UID, so Gradle's downloads are included without any socket tagging -- the Gradle Tooling and daemon processes share it. There is deliberately no per-feature breakdown; the only two tagged sockets in the tree are the local documentation web server and the JDWP listener, neither of which is interesting here. The platform counters are cumulative since boot, so NetworkUsageWatcher records the delta between consecutive samples. Three cases the raw counters would get wrong: - The first sample only establishes a baseline and contributes 0. Otherwise the chart would open with a spike equal to everything the app had transferred since boot. - A counter that goes backwards (reboot, re-based accounting) records 0 rather than plotting negative traffic. - TrafficStats.UNSUPPORTED (-1), which some devices return, is detected once and latched, so -1 is never plotted as a byte count. getUsage() hands out copies rather than the live ring buffers, guarded by a lock. The renderer reads all 30 entries while the sampler thread appends, and MemoryUsageWatcher's equivalent has that race today. Axis, per the ticket's decisions: - Values are log10(bytes + 1). Traffic spans orders of magnitude -- a few hundred bytes of chatter next to a multi-megabyte download -- and a linear axis flattens all of it but the largest burst onto the baseline. MPAndroidChart has no logarithmic axis. - The + 1 floors zero, which is the common sample rather than an edge case: an idle IDE transfers nothing and log10(0) is negative infinity. A zero sample plots at exactly 0.0 and the line stays continuous. - Units are decimal (1 kB = 1000 B), not binary. This was not in the ticket and is a consequence of the log axis: on-device the first cut labelled the gridlines 9B / 99B / 999B / 9.8KB, because powers of ten divided by 1024 stop looking like decades. Decimal units label them 0B / 10B / 100B / 1.0kB, and are the convention for throughput. - Axis labels show 10^value rather than the exact inverse 10^value - 1, which would read 9B / 99B / 999B. One byte is not worth the confusion, and the legend carries the exact current figure. Zero is labelled exactly, since log10(0 + 1) really is 0. MetricsPage.Image and its layout go with the placeholder, having no remaining user; ADFA-5490 will define its own extension surface. The cogo_brand_mark drawable stays -- six other screens use it. Verified on a Pixel 6 Pro (arm64), v8 debug, over wifi with a real Gradle sync: - Both series track real traffic (peaks ~10kB/s against byte-level chatter, both legible on the one scale), idle periods sit flat on the 0B baseline, and the axis reads 0B / 10B / 100B / 1.0kB / 10.0kB. - Swiping to the memory page and back returns the full 30-sample history, so the page is recycling-safe like the memory one. - Font scale 1.0 and 2.0, measured on a cold start (EditorActivityKt declares fontScale in configChanges, so a warm relaunch reports stale geometry): title 22dp -> 35dp, pager 185dp -> 171dp, panel 248dp throughout, nothing clipped. - Landscape renders correctly, nothing clipped. - 16 new tests (7 watcher, 9 renderer); 80 tests green across app ui/utils/activities/fragments. ADFA-5489 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
Two axis problems, one cosmetic and one a real rendering bug. Labels now read "10 kB" rather than "10.0kB". Gridlines sit on whole decades (granularity 1), so the mantissa is always exact and the decimal place carried no information. formatBytes takes the precision as an argument: none for axis labels, one place for the legend, where the figure is an arbitrary sample and the decimal does carry information. A space separates value from unit throughout. Zero now rests on the baseline. Two causes, both fixed: - The series were scaled against the wrong axis. LineDataSet defaults to axisDependency LEFT, and the labelled axis here is the right one, so the line was positioned by the disabled, auto-ranged left axis while the labels came from the right. The two only agree while both auto-range over the same data; pinning one made them disagree visibly -- an idle chart drew its zero line halfway up a plot whose baseline was labelled 0 B. - The range was not pinned. With every sample zero the data range is degenerate and the chart pads around it. applyAxisRange now fixes the minimum at 0 and the maximum at whole decades above the peak, with a floor of three decades so an idle chart keeps a sensible scale instead of collapsing onto a single value. Worth noting for review: the unit tests asserting axisMinimum and axisMaximum passed throughout, because the axis really was configured correctly -- the data simply was not drawn against it. Only the device showed it. There is now a test asserting the axis dependency of both series, which is the part that was untested. MemoryUsageChartRenderer has the same LEFT-dependency-with-RIGHT-labels shape and renders correctly, because it pins neither axis and both auto-range over the same data. Left alone. Verified on a Pixel 6 Pro (arm64), v8 debug: - Idle: both series rest exactly on the 0 B baseline, axis reads 0 B / 10 B / 100 B / 1 kB. - Under a Gradle sync: axis grows to 10 kB, peaks and zero-traffic troughs both legible, legend reads "212 B/s". - 49 tests green across app ui/utils, including four new ones covering the axis range, its growth across both series, whole-unit labels, and the axis dependency. ADFA-5489 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
The sampling loop called a hardcoded delay(1000), ignoring the updateInterval constructor parameter it was given. Passing a different interval changed nothing, so the sample rate was fixed at one second whatever a caller asked for. NetworkUsageWatcher (ADFA-5489) uses its interval correctly, so the two watchers disagreed. This is the "sample time is fixed" of ADFA-5486, present in the code and not only in the UI. Making the interval configurable from settings is the rest of that ticket; this makes the existing parameter mean something first. Two supporting changes, both needed to test the loop at all: - The dispatchers are injectable, defaulting to the single-thread context and Dispatchers.Main.immediate as before. Tests drive the loop on a TestDispatcher and advance virtual time, so the regression test is deterministic rather than a wall-clock race. A first attempt that slept on the real clock hung the test executor. - readUsages() returns before the ActivityManager lookup when no process is being watched. Behaviour-preserving -- it went on to iterate zero pids -- and it keeps an idle watcher off BaseApplication, which a unit test does not have. Verified the tests fail without the fix: with delay(1000) restored, "the sampling rate follows the configured interval" reports 1 sample where it expects at least 9, for exactly the reason it is named for. The longer-interval test passes either way by construction; it guards the proportionality, not the bug. Verified: :app:testV8DebugUnitTest, 51 tests green across app ui/utils. ADFA-5486 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
ADFA-5489 gave the metrics carousel a second chart, and with it a second copy of the chart setup: the two renderers had a byte-identical configure() apart from the value formatter, and a byte-identical block in rebuild() applying theme colours and redrawing. ADFA-5486 adds x-axis labels, zoom, event annotations and snapshot export to "the line chart", written when there was only one. All four belong on both charts, and duplicated setup is how they end up on one. This puts the common behaviour in one place before that work starts. MetricsChartRenderer holds the attach/detach lifecycle -- including detachIfAttached, which a recycling carousel page needs -- the shared axis and gesture configuration, and the data/redraw helpers. Subclasses override configure() to add what is theirs (the memory chart's MB formatter; the network chart's byte formatter and per-decade granularity) and call through. Behaviour-neutral: no configuration value changed, only where it lives. The existing renderer tests are the evidence, and both charts were compared on device against the previous build. Verified: :app:testV8DebugUnitTest, 51 tests green across app ui/utils; both carousel pages rendered on a Pixel 6 Pro (arm64, v8 debug), including the network chart under a live Gradle sync. ADFA-5486 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
…ndow Retention goes from 30 samples to 3600 -- an hour at the current one second interval -- so that zoom, pan and event annotations have something to work against. Against 30 samples they are close to meaningless. Three parts, each a consequence of the first: History moves into MetricsViewModel. The watchers were fields on the editor activity and survived rotation only because EditorActivityKt happens to declare orientation in its configChanges. Drop that flag, or add a screen that does not declare it, and an hour of history would vanish silently. An activity-scoped ViewModel makes survival a property of the lifecycle rather than a manifest coincidence. It does not survive process death; that is ADFA-5494. The chart shows a window of 60 samples rather than all 3600. Holding an hour is cheap -- about 29KB of longs per series -- but drawing 3600 points per series into a 200dp strip is not, and it would be illegible anyway. MPAndroidChart clips drawing to the visible x range, so a window keeps the cost independent of how much is retained. This is also the shape the zoom feature needs, arrived at from the other direction. The x axis is labelled by age. Sample indices were already meaningless and would now run to 3599. This pulls forward part of the ticket's x-axis-labels step, because 3600 samples made the old labels actively worse rather than merely uninformative. Two bugs found on device that no unit test would have caught: - A bound callable reference evaluates its receiver where it is written. Passing memoryUsageWatcher::getMemoryUsages from a field initializer therefore reached the ViewModel during the activity constructor, which throws "You can't request ViewModel before onCreate call" and made the editor unlaunchable. The providers are lambdas now, so the watcher is resolved per call. - The visible x range is held as a scale factor, so a layout change left the window pointing at a different part of the history: after a rotation the chart showed samples from half an hour earlier, with the axis reading -1979s. The window is re-applied on every redraw rather than only when data is set. Verified on a Pixel 6 Pro (arm64), v8 debug: - Both charts show a rolling 60-second window, x axis reading -59s to now, over an hour-deep buffer. - History survives rotation: the same traffic burst was still on screen after a portrait/landscape round trip, correctly aged from -14s to -29s, with sampling continuous across the change. - Landscape re-verified after the viewport fix; no crashes throughout. - 66 tests green across app ui/utils/activities. Known and deliberate: sampling still stops in onPause, so a backgrounded editor leaves a gap that the evenly-spaced x axis does not represent. Raised on the ticket. ADFA-5486 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
Two things, both from looking at the device rather than the tests. The x axis labels were never missing. MPAndroidChart defaults every component's text to Color.BLACK. setData gave the y axis and the legend a themed colour and nobody ever gave one to the x axis, so its labels have been drawn black on a near-black surface for as long as the chart has existed. Brightening a screenshot 3.2x shows them sitting there perfectly well formed. That is the "the line chart x axis has no labels" of ADFA-5486: not absent, invisible. One line fixes it. Sampling now continues while the editor is backgrounded. It used to stop in onPause, which was harmless at 30 samples and is not at 3600: the x axis assumes samples are evenly spaced, so any spell in the background made it misreport how old everything to the left of the gap was. Only the listeners are dropped on pause, so nothing redraws a chart nobody is looking at, and sampling itself now lives as long as MetricsViewModel. onResume rebuilds both charts rather than waiting a tick, and only starts a watcher that is not already running -- otherwise every resume logged a spurious "already being watched" warning. This also makes the chart answer a question it could not before: what memory did while you were not looking. Verified by backgrounding the editor for 25 seconds -- the chart came back showing the drop as the app went away, the plateau while it was gone, and the rise on return, all recorded. Battery: one /proc read and one TrafficStats read per second while backgrounded. Modest, and the platform freezes cached processes anyway, which stops it for free. Verified on a Pixel 6 Pro (arm64), v8 debug: - x axis reads -59s / -44s / -29s / -14s in the same colour as the y axis labels. - Background sampling as described; no gap in the history. - No "already being watched" warnings in logcat; no crashes. - 66 tests green across app ui/utils/activities. ADFA-5486 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
Groundwork for the tap-on-x-axis rate dialog, which the ticket description now specifies (0.1s to 60s). The dialog itself is not built yet; this is the machinery it will drive. Retention goes from 3600 to 10000 samples. With the rate variable, a sample count no longer means a fixed span: 10000 covers most of three hours at one second and about seventeen minutes at the 0.1s floor. 80KB of longs per series, and drawing cost is unchanged because the chart shows a window rather than the whole buffer. The sampling interval is now settable, and changing it clears the history. The chart reads a sample's age from its position, which assumes every sample is the same age apart; a buffer holding samples taken at two rates would silently misdate all the older ones. The network watcher also drops its cumulative baseline, otherwise the first sample after a change would report every byte since the previous one as a single delta -- a spike at exactly the moment the user changed the rate. MetricsSamplingRates holds the floors: 0.1s on 64-bit hardware, 0.5s on 32-bit. Sampling costs a Debug.getMemoryInfo call per watched process plus two TrafficStats reads every interval, and ten times a second on a weak device is enough to distort what the chart is measuring. Rates a device cannot use are still listed, marked unavailable, rather than hidden -- Rate.isAvailable is what the chooser should grey out. A chooser that silently omitted them would leave the user assuming the IDE cannot sample faster, rather than seeing that their hardware is what costs them the two fastest rates. The floor is keyed on the device's architecture, not the build flavour: a 32-bit build of the IDE running on a 64-bit phone is still running on hardware that can afford the faster rate. Verified on a Pixel 6 Pro (arm64), v8 debug: both charts render unchanged at the higher retention, no crashes. 60 tests green across app ui/utils, 9 of them new. ADFA-5486 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
The carousel's pages, renderers, page-change callback and watcher listeners were spread across BaseEditorActivity. Undocking (ADFA-5486) needs the same carousel built against a floating window's context, so running one is now a thing an object does rather than something an activity is. The activity keeps what is genuinely its own: the status-bar inset on the pager, when to start and stop sampling, and which colour each watched process is drawn in -- the last passed in as a lambda, because the process names it keys on belong to the activity. Binding also takes over the watcher listeners, which is what makes the controller the single owner of "a carousel that is being looked at". onPause unbinds and onResume rebinds; sampling is untouched by either, so the history stays continuous. Worth recording for the undocking work: only one carousel can be live at a time. MemoryUsageWatcher and NetworkUsageWatcher each hold a single listener, not a list, so a second carousel would silently take the updates from the first. Undocking therefore has to move the carousel out of the editor rather than copy it into the window -- which matches how an editor file tab already undocks, leaving the tab row. Behaviour-neutral. Verified on a Pixel 6 Pro (arm64), v8 debug: both pages render, paging works, and backgrounding for 15 seconds and returning shows continuous history across the gap, exercising the unbind/rebind path. 75 tests green across app ui/utils/activities. ADFA-5486 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
A two-finger tap on the carousel floats it over other apps, and the editor shows "Metrics are in a floating window. Tap to bring them back." in the space it vacates. Tapping that message, or the window's own dock control, brings it back. Undocking moves the carousel rather than copying it. MemoryUsageWatcher and NetworkUsageWatcher hold a single listener each, so two live carousels would mean the second silently taking the first one's updates. MetricsCarouselDockableContent therefore rebinds the editor's own MetricsCarouselController into the window, and the editor shows the message instead. That also matches how an editor file tab undocks, leaving the tab row. The history is untouched by the move: the watchers own it, so the carousel is redrawn in full wherever it binds. Without the message the reveal would open on an empty strip, which reads as broken, and a window dragged off screen would leave no way back. The gesture is recognised in dispatchTouchEvent, not onInterceptTouchEvent. ViewPager2's RecyclerView calls requestDisallowInterceptTouchEvent on its parents the moment a second pointer lands, and a ViewGroup only calls onInterceptTouchEvent while that flag is clear -- so the first version saw the two fingers arrive and never saw them leave. It fired on nothing. dispatchTouchEvent is delivered first and the flag does not affect it. The unit tests did not catch that, because they called onInterceptTouchEvent directly: they proved the recogniser's logic and not that the framework would ever call it. They now drive dispatchTouchEvent, which is what actually happens. Same failure as the chart axis earlier in this ticket -- a green test over a wire that was never connected. Also generalises the project-close teardown. closeAll released resources only for EditorPanelDockableContent, so any other content type would be removed from DockingManager without being told; it now gets onDestroyView, which is how the carousel unbinds its controller. Verified on a Pixel 6 Pro (arm64), v8 debug, the two-finger taps done by hand because adb cannot inject multi-touch and sendevent needs root: - Two-finger tap undocks; the window shows the carousel with its chrome and the editor shows the message. - Tapping the message re-docks, and the chart returns with its history intact across the float. - FloatingTabService starts on undock and stops on re-dock; no leaked service, no crashes. - 508 tests green across the app module, 5 of them new for the gesture. Known gap: the two-finger tap cannot be exercised in CI for the same reason it could not be scripted here. ADFA-5486 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
…489-network-traffic-page
Three defects raised in review of ADFA-5487/5489, all in the same few lines and all present in both watchers. stopWatching() could not stop the sampler. The loop was launched with `launch(context = SupervisorJob() + dispatcher)`, which gives the coroutine its own parent job, so the watcher's scope could not cancel it: it ran on until it next observed the `watching` flag, and it spends almost all of its time asleep in `delay(updateInterval)`. Stop and start inside that window and the old loop woke up, saw the flag set again, and carried on beside the new one -- two samplers writing history and notifying the chart. The window is as wide as the interval, which ADFA-5486 made configurable up to sixty seconds. The job is now stored and cancelled. An exception ended sampling permanently. A throw anywhere in the body killed the coroutine while `watching` stayed true, so every later startWatching() was refused as "already watching" and the chart silently stopped updating for the rest of the session. A misbehaving listener was enough. The body is guarded now: a sample is worth losing, the loop is not. CancellationException is rethrown so cancellation still works. The dispatcher was never closed. `newSingleThreadContext` holds a thread until closed, and nothing closed it. close() is separate from stopWatching() because the watcher is stopped and restarted across the editor's lifecycle; only the terminal teardown should give up the thread. MetricsViewModel.onCleared calls it. startWatching() also uses compareAndSet rather than a check followed by a set, so two callers cannot both pass the guard. Tests: 5 new lifecycle tests. Verified they fail without the fix, though the first one fails by hanging rather than by asserting -- with the loop unstoppable, runTest never drains the scheduler. That is the bug seen from the inside, and it is why each test now closes its watcher. Verified on a Pixel 6 Pro (arm64), v8 debug: chart samples continuously across a background/foreground cycle, no crashes, nothing logged from the new failure guard. 70 tests green across app ui/utils. Addresses CodeRabbit findings on #1784. ADFA-5486 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
Significant events are Gradle task starts and stops, drawn as dashed vertical markers labelled with the task name. Gradle emits those far faster than a chart can show them -- an incremental build blasts through dozens of up-to-date tasks in a second or two -- so MetricsAnnotationStore throttles to at most one every five seconds and keeps the first of each quiet period, since the interesting moment is when work began rather than an arbitrary one from the middle of a burst. Annotations are stored by wall-clock time, not by sample position. The charts hold a ring buffer whose contents shift under them, so a stored index would drift; the renderer converts a timestamp to an x position from its age at draw time, and anything older than the buffer holds falls outside the axis. A marker therefore travels left with the data and leaves the visible window, which is what it should do. The events already reached EditorBuildEventListener.onProgressEvent for the status line, so this needed no new plumbing -- only a second use of the same TaskStartEvent, plus TaskFinishEvent. Worth recording, because it would have shipped silently broken: lastRecordedAt started at Long.MIN_VALUE, so `now - lastRecordedAt` overflowed to a negative gap on the very first call. That reads as "inside the throttle window", so the store swallowed every annotation for its entire life and nothing anywhere reported an error. All seven tests caught it on their first run. It is nullable now. Verified on a Pixel 6 Pro (arm64), v8 debug: a project sync records nothing, correctly -- a sync configures and emits no task events -- and a build draws a marker at :app:preBuild, confirmed on screen. The rest of that build's tasks completed inside the five-second window and were collapsed into that one marker, which is the throttle working as specified. 7 new tests; 77 green across app ui/utils. ADFA-5486 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
Long-pressing the chart title writes the visible chart to a PNG and hands it to the system share sheet, so it can go into a ticket, a chat or a file. Snapshot means an image of the chart, as decided on the ticket. The gestures over the chart itself are all spoken for -- paging, panning a zoomed chart, and the two-finger tap that undocks -- so the title is the target: an unambiguous one that behaves the same whether the carousel is docked or floating. Images go to a directory under the cache, so the platform can reclaim them, and each export clears the previous one. This is a scratch space for handing a single image to another app, not a gallery; the sharing intent grants the receiving app access before the next export matters. Chart titles are translated, so the filename is derived rather than copied: lowercased, everything outside a-z0-9 collapsed to hyphens, and falling back to "metrics" if nothing usable is left. Verified on a Pixel 6 Pro (arm64), v8 debug: long-pressing the title raised the share sheet showing a preview of the real chart, and left memory-usage-20260905-103613.png (37KB) in the cache directory. 5 new tests; 82 green across app ui/utils. ADFA-5486 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
A tap on the x axis opens a chooser offering every rate from 0.1s to 60s, as the ticket specifies. Picking one applies it to both watchers and discards the history, because a buffer holding samples taken at two rates would misdate the older ones. Rates the device cannot use are listed and greyed rather than hidden, so the user can see that their hardware is what costs them the two fastest rates instead of assuming the IDE cannot sample faster. On a 64-bit device all nine are selectable; on 32-bit the 0.1s and 0.2s entries read "needs a 64-bit device" and do nothing. The tap is recognised through the chart's own gesture listener rather than a view: the axis is drawn by MPAndroidChart, so there is nothing to attach a click listener to, and only the chart knows where it put the axis. A tap above viewPortHandler.contentTop landed on it. Two bugs found on the device while doing this: The dialog first appeared with no list at all. An AlertDialog shows either a message or a list, never both, and the message silently wins -- so the explanatory line had swallowed the nine rates. The explanation lives on the greyed entries instead. The x axis kept labelling with the old interval after a rate change: ElapsedTimeFormatter captured sampleIntervalMillis at construction, so at 5s per sample it still read -54s where the leftmost sample was really 295 seconds old. Annotation positioning shared the flaw. Both take a provider now and read the live value. I had flagged this risk when making the interval settable and then did not carry it through. Verified on a Pixel 6 Pro (arm64), v8 debug: the chooser opens from an axis tap with the current rate ticked, selecting a slower rate clears the history and refills at the new rate, and the gridlines re-space to match. 82 tests green across app ui/utils. ADFA-5486 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
Replaces the long-press on the chart title with a camera button in the graph's bottom-right corner, at your request. The long-press worked but advertised nothing: a user had no way to discover that the title did anything. A visible control does not have that problem, and it costs no gesture -- every gesture over the chart is already taken by paging, the two-finger tap that undocks, pinch to zoom, and the tap on the x axis for the sampling rate. The icon is small, as asked, and sits as low and as far right as the graph area allows. The button around it keeps a 40dp touch target, since the visual size of a control and its touch target need not match, and a 24dp target would be hard to hit. Verified on a Pixel 6 Pro (arm64), v8 debug: the button sits in the corner of the plot, and tapping it raises the share sheet showing the real chart, leaving memory-usage-20260905-105005.png in the cache. ADFA-5486 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
… axis The time axis zooms and a zoomed chart pans, without taking the swipe that pages the carousel. The x axis moves to the bottom of the plot. Your split -- carousel swipe below the axis, pan above it -- assumed the conventional position, and ours was at the top, where "above the axis" is a sliver against the status bar. At the bottom the split describes real regions: the plot, and the strip of axis labels, legend and title beneath it. Ownership of a horizontal drag is settled once, on the way down, before either the pager or the chart has seen a move: the pager's touch paging is switched off for the gesture when the drag starts inside the plot of a zoomed chart, which lets the drag through to pan it. Everywhere else the carousel keeps the swipe -- the strip below the axis always, and the whole chart while it is at rest, since there is nothing to pan to. Only the time axis scales. Zooming the value axis on a memory or throughput chart just makes the numbers lie about their own scale. Two things that would otherwise make zoom useless: the auto-follow window no longer re-centres while zoomed, which would have dragged the user back to the newest samples once a second; and switching carousel page resets the zoom, so a page left magnified does not go on claiming horizontal drags when it comes back. Not verified on hardware. A pinch cannot be injected on an unrooted device -- adb input has no multi-touch and sendevent needs root -- which is the same limit the two-finger tap hit. The axis position and the absence of regressions are verified; the pinch itself needs a hand. 82 tests green across app ui/utils. ADFA-5486 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
…g arrows Two of the three problems reported from the device turned out to be one bug. Showing a 60-sample window of a 10000-sample buffer *is* a zoom as far as MPAndroidChart is concerned: scaleX sits around 166 at rest. So testing `scaleX > 1f` for "has the user zoomed" was always true, with two consequences. The auto-follow window stopped re-centring after the first draw, which is why a floating window drifted to around -5000s. And the chart claimed every horizontal drag, which is why moving between carousel pages was so hard -- the swipe was being taken to pan a chart nobody had zoomed. Zoom is now recorded from the scale gesture itself rather than inferred from the viewport, which cannot be confused by the window we set. Paging arrows either side of the chart title. Swiping still works, but it competes with panning a zoomed chart and with the editor's drawer gesture, and losing that race intermittently is worse than not having the gesture at all. The arrow for an end of the carousel is dimmed and disabled. Keyboard in the floating window: nothing in the carousel is typed into, so nothing in it should take focus. A focusable child makes an overlay window focusable, and the soft keyboard then opens over the chart on every touch. The content blocks descendant focus, and a touch also dismisses any keyboard already showing. Verified on a Pixel 6 Pro (arm64), v8 debug: the arrows move between pages and dim at each end, and the axis, window and legend are unchanged otherwise. The keyboard fix and the floating-window drift need the window open to confirm. 82 tests green across app ui/utils. ADFA-5486 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
The shared arrow drawables carry a hardcoded android:tint="#000000", so the paging arrows were drawn black on the near-black chart surface and could not be seen at all. This is the same failure as the x axis labels earlier in this ticket, which were invisible for the same reason -- MPAndroidChart defaults its text to Color.BLACK -- and it happened again because these icons were reused without checking what colour they came with. Anything drawn on this surface needs its colour asserted at the usage site rather than assumed. Tinted at the usage site rather than by editing the shared drawables, which are used elsewhere on light backgrounds. Verified on a Pixel 6 Pro (arm64), v8 debug: both arrows legible, the one at the end of the carousel dimmed. ADFA-5486 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
Swiping in the graph area no longer changes page. The arrows either side of the title are the only way. This removes a three-way contention rather than arbitrating it. A horizontal drag in the plot was wanted by the carousel, by a zoomed chart wanting to pan, and by the editor's drawer gesture; deciding between them per gesture worked, but losing the race intermittently made the carousel feel unreliable, and no amount of tuning makes an ambiguous gesture feel deliberate. With touch paging off, a horizontal drag in the plot is unambiguously a pan, and paging is a plain control that cannot be misread. The gesture arbitration goes with it: the router in MetricsCarouselLayout, the paging-enabled callback, and handlesHorizontalDragAt on the renderer are all deleted rather than left switched off. What stays: the layout still asks its ancestors not to intercept, so a horizontal drag in this strip reaches the chart to pan with instead of opening the drawer, and the editor's fling detector still excludes the carousel's bounds. The x axis stays at the bottom. It moved there so the strip beneath it could be reserved for the carousel swipe, which no longer exists, but the bottom is the conventional place for a time axis and moving it back would be churn. Verified on a Pixel 6 Pro (arm64), v8 debug: a swipe across the plot leaves the title on "Memory usage", and the next arrow moves it to "Network traffic". 82 tests green across app ui/utils. ADFA-5486 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
…FA-5486-chart-improvements
…-5499) A third carousel page charts battery temperature against instantaneous power draw, with thermal throttling shaded behind the plot and the battery level shown in the corner. Design decisions, and what was rejected: - Instantaneous power, not cumulative. A running total only ever rises and says nothing about which piece of work cost anything; instantaneous draw lines up with the spikes on the memory and network pages. - Battery readings only. The per-zone CPU, GPU and skin temperatures need android.permission.DEVICE_POWER, which is prot=signature|role|module -- it cannot be granted to an installed app, so there is no prompt to defer and no fallback worth attempting. PowerSource is an interface so a privileged build can supply better readings without the chart changing. - Throttling is shaded, not plotted. The platform reports an ordinal level, not a temperature, so plotting it against degrees would invent a scale. Alpha rises with severity so the bands read as a gradient of concern. - Battery level is a readout, not a series: it moves about a percent every few minutes, so over the chart's window a line would be flat, spending an axis on a constant. Hidden while charging, when a rising level would contradict a chart about power being spent. Charging periods are not shaded. - Two value axes, the only page with them. Degrees and milliwatts share no unit, so each series declares its axis; a series left on the default would be drawn against labels that do not describe it. - Power is plotted as a magnitude. The battery current reverses while charging, and a line dipping below zero would read as negative power spent. Two defects found on-device, both invisible to passing unit tests -- the same class of failure as the black-on-black axis labels and the black-tinted arrows earlier in this stack: - Shading never reached the screen. setDrawGridBackground(true) fills the plot opaquely inside super.onDraw, so spans painted before it were covered. SafeLineChart now overrides drawGridBackground and paints the spans straight after that fill, which also puts them under the grid lines and the data. - A single-sample throttle had zero width. Spans ran centre to centre, so one sample mapped to one pixel column and two adjacent runs left a sample-wide gap. Each span now covers its samples' full cells. Also wired up two things that were built but unreachable: the power page's x-axis tap now opens the sampling-rate chooser like the other pages, and batteryReadout() now has a view to write to. Verified on a Pixel 6 Pro (arm64) with `cmd thermalservice override-status` stepped through levels 1, 3 and 6 and `dumpsys battery unplug`: three bands appear, deepen with severity, abut without gaps, and stop when the override clears. Checked at font scale 1.0 and 2.0 -- the title, arrows and battery readout all grow without clipping. The chart's own axis and legend text is drawn by MPAndroidChart in dp and does not scale, which is a pre-existing limitation of the library recorded under ADFA-5486, not new here. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
…er annotations Four changes to the temperature and power page (ADFA-5499) and one to the annotations shared by every page (ADFA-5486). Throttle shading is now hue-coded rather than one colour at six depths: green, cyan, yellow, orange, rust, red for levels 1 to 6, at one fixed alpha. Level 0 and an unreadable level stay unshaded. Ranking seven ordinals by depth of a single colour asks the eye to compare shades that are never side by side; the bands are separated in time, so distinct hues stay tellable apart wherever on the chart they fall. The source is unchanged: PowerManager.getCurrentThermalStatus() on API 29+, with ThermalInfo behind it for API 28, which minSdk still admits. The power axis is labelled in whole watts. A build peaks in single-digit watts, so milliwatt labels spent three characters each on trailing zeros. Granularity is pinned to 1 W as well: left to choose its own spacing the axis puts gridlines a fraction of a watt apart on an idle device, and rounding those to whole watts prints the same label several times over. The legend keeps finer units, falling back to milliwatts below a watt, where "0W" would lose the only value it exists to show. Each value axis takes the colour of the line it describes -- orange for temperature on the left, blue for power on the right. With two axes carrying unrelated units, colour is what says which reads which. That last one needed a hook. setData repaints both axes in the surface's text colour on every redraw, so anything a subclass set in configure was overwritten within a frame; it now calls an open styleValueAxes, which the power page overrides. The test caught this -- the same shape as the two defects in the previous commit, and this time it was caught before the device. Annotation labels are staggered across eight rows, cycling. Gradle fires tasks in bursts, so several markers land within a few pixels of each other and their labels, all drawn on one row, overwrote each other into an unreadable smear. The row comes from a new Annotation.sequence, counted from the first annotation of the session, rather than from a position in the visible list: that list shifts as older entries age out, so a label would hop rows while merely sitting still. Nothing covered the drawing of annotations before this, only the store behind them, which is how the smear came to ship. MetricsAnnotationRenderingTest now covers it; its three stagger tests were confirmed to fail with the offset held constant, and the row-stability test to fail when the row is taken from the visible list. Verified on a Pixel 6 Pro against a newly created Compose Activity project, so the Gradle run was long and task-dense: three annotations drawn on three different rows, the right axis reading 0W through 6W, the left axis orange and the right blue, and all six throttle hues distinct under `cmd thermalservice override-status`. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
…wn on (ADFA-5486) The chooser was reachable only from a blank strip above the plot, at the opposite end of the chart from the axis labels the gesture is named for. The hit test compared against contentTop while the axis is positioned BOTTOM, so tapping the labels did nothing and the rate could not be changed by anyone who did not already know where the hidden band was. The strip under the plot had been left alone for the carousel swipe. Paging is by the arrows now, so it is free, and the tap moves there. The two have to agree, and nothing said so: a comment on each site now points at the other. MetricsChartAxisTapTest covers all three bands. Confirmed to fail against the old hit test in both directions -- the tap below the plot not registering, and the tap above it still registering -- so it pins the edge rather than merely the existence of the gesture. A guard test asserts the chart was laid out first, without which every coordinate sits on the same edge and the others would pass vacuously. Verified on a Pixel 6 Pro: tapping the "-54s" labels opens the chooser, tapping the band above the plot does nothing, and picking "Every 5s" relabels the axis to -270s and clears the history as intended. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
hal-eisen-adfa
left a comment
There was a problem hiding this comment.
One finding from a review pass, inline. (A second finding, about GradleDaemonWatcher.shutdown() never being called, turned out to pre-date this branch, so it is a separate top-level comment rather than an inline one.)
|
Flagging it on this PR because this branch is the one that consolidates the daemon-watching work, but the code already exists in the base commit (
Consequences:
Calling |
detach() cleared userHasZoomed and appliedTextScale but not reservedTopPixels. The reservation is memoised per chart, so carrying it across a detach meant a rebind onto a fresh SafeLineChart asking for the same inset took reserveTopSpace's early return and never got setExtraTopOffset at all -- and nothing else applies it, unlike the text scale, which setData re-applies on every rebuild. The battery readout then covered the right axis's topmost label again, which is the whole reason the inset exists. Reachable by undocking the power page and docking it back. MetricsCsvFile wrapped only the outer sink in use(), so a GZIPOutputStream constructor that throws -- it writes the gzip header there -- leaked the FileOutputStream it had already been handed. That is the crash-attachment path, so a device whose cache cannot be written leaked a descriptor per reported event. The raw stream now has its own use(). The rebind case has a regression test; removing the reset fails it. The leak does not: making the gzip header write fail needs a fault injection seam this class does not have, and adding one for it seemed a worse trade than the four-line change. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
The strip is a fixed editor_mem_usage_view_height and the undocked message fills it at 0dp/0dp with nowhere to scroll, so the only thing keeping it readable at a 2.0 font scale is that it still fits. Nothing measured that. It does fit -- 140px of the 496px it has, at 2x on xhdpi -- and now a test says so. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
The sampling loop copied the map but handed the listener the live ProcessMemoryInfo objects, so the renderer read all 3600 slots of the live ring buffer on the main thread while the sampler was mid-append -- it could see the advanced shift against the not-yet-written value and plot every point one slot out of place. That is the failure getMemoryUsages() snapshots to prevent (ADFA-5531); the listener path was bypassing the discipline this same PR added. It snapshots under historyLock now. The thermal series was filled and cleared with UNAVAILABLE (Long.MIN_VALUE) while its own KDoc, every consumer and MetricsSnapshotAssembler's `absent` all use THERMAL_UNKNOWN (-1). It rendered unshaded only by accident: Long.MIN_VALUE.toInt() is 0, which is THERMAL_STATUS_NONE -- "measured and not throttled" -- and the CSV would have written the raw MIN_VALUE into the thermal_status column, the spectacular-wrong-answer case Series.absent exists to prevent. `history is all zeros before the first sample` did not catch that, because it asserted sum() == 0 and 3600 * Long.MIN_VALUE wraps to exactly 0. The assertion held whether the buffers carried the sentinel, zeros, or the wrong sentinel. It asserts per slot now. markerRows walked every marker against every sampled row -- the O(markers x rows) cost its own KDoc says it avoids, ~2.6M compares at a full buffer, on the thread that just threw -- and boxed a 3600-element IndexedValue list to do it. Binary search over two parallel arrays. warnIfProcessHasGraphicsMemory line-scanned /proc/<pid>/maps on the caller's thread, and for the Gradle daemon that caller is a main-dispatched build callback: a StrictMode DiskReadViolation and visible jank in the build a developer is watching, for a debug-only warning. Moved to its own IO scope. DevicePowerSource re-fetched the sticky ACTION_BATTERY_CHANGED Intent per sample with registerReceiver(null, ...) -- a binder round trip to the system server, up to ten a second, to re-read values that move on the order of seconds. One registered receiver caches the last Intent. metricsAttachmentForFeedback forced MetricsSnapshotAssembler onto the main thread, though it is @anythread precisely because a crash arrives on whatever thread threw. It allocated eleven LongArray(3600) and copied 39,600 longs there while contending for three sampler locks. Corrected two claims rather than the code: EventListener's KDoc said the daemon callbacks were "Defaulted" when the declaration has none and GradleBuildServiceListenerWrapperTest forbids one; MetricsScratch claimed snapshotting "needs no memory" when the write path it feeds still takes a Deflater, a writer buffer and a String per cell. Not fixed here, and why: - onPause still samples while backgrounded. Review argued the "evenly spaced samples" justification is dead now that sampleTimes exists. It is not: ElapsedTimeFormatter positions every point as (newestIndex - value) * sampleInterval, so a gap still misreports ages on the chart, which is ADFA-5486's original bug. Only the CSV carries real timestamps. Stopping while backgrounded needs the timestamp-based x axis first, which is ADFA-5596. - The per-tick rewrite of all 3600 entries is real but cannot be narrowed to the visible window: the history is a shifted ring buffer, so every index's value changes on append and a windowed update would leave the panned region stale. - The per-tick getUsage() copy is likewise not a simple win: the copy is what makes the hand-off to the main thread safe, which is the same reason the snapshot above was needed. Removing it needs double buffering. Those three are on ADFA-5619 with this reasoning. Tests: full app unit suite green, spotlessCheck clean. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
…receiver getMemoryUsages() read memoryUsage.size and then values.elementAt(index) per element. watchProcess(unique = true) removes an entry, and it runs from the tooling server's own thread and from a CompletableFuture completion -- so a removal between the two threw IndexOutOfBoundsException on the main thread, from inside a chart redraw. One walk over the values now, which is also O(n) rather than O(n^2). watchProcess, unwatchProcess and unwatchAll take historyLock, and readUsages appends to every watched process rather than only the ones it read. Taking the lock alone was not enough: a registration that blocked until the append finished still started one append behind sampleTimes, which is the same permanent misalignment. A process registered mid-read gets a zero for the sample it was not present for, which watchedSinceMillis already tells the exporter to blank. DevicePowerSource.close() had no caller, so the receiver registered in its constructor against the application context outlived the watcher and the editor: one more receiver per editor session, for the life of the process. That was a regression from the previous commit, which added the receiver to remove a per-sample binder call and never wired its teardown. PowerUsageWatcher.close() now closes an AutoCloseable source, and PowerSource stays a fun interface so tests can still pass a lambda. MetricsCarouselDockableContent.onDestroyView() unbound the shared controller unconditionally. The undock and redock paths are two independent collectors of the same DockingManager emission with nothing ordering them, so the editor can rebind before the window tears down, and the unbind then stripped the editor's own carousel. Now identity-guarded through unbindIfBoundTo, like every sibling teardown here. setSamplingInterval cleared the annotation store unconditionally, but the watchers' setters return early on an unchanged value -- so re-picking the rate already in effect, which the dialog allows, wiped every build marker off a chart whose samples were untouched. MetricsScratch sizes its arrays from the largest of the three retentions rather than the memory watcher's alone. They agree today, and ShiftedLongArray.copyInto require()s an exact match, so the day they stop agreeing the throw lands where runCatching swallows it and every crash report silently loses its metrics. Tests: full app unit suite green, spotlessCheck clean. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
The recycle sat in a finally inside withContext(Dispatchers.IO), so it was only reachable once that block had started running. The bitmap is taken before the launch, on the UI thread, and close() cancels the scope on undock and on activity destroy -- a cancellation at that suspension point skipped the finally and left a full-size ARGB_8888 copy of the plot, the largest thing this class allocates, to the collector. Tap the camera and immediately undock to reproduce. The finally now wraps the whole launch body. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
GradleDaemonWatcher.shutdown() had no caller. Reported by hal-eisen-adfa on #1812, where the code is in the base commit rather than the diff, so it lands here instead -- this is the file that owns the watcher. Three consequences, all his: - the watcher's thread outlived server shutdown, and an in-flight poll chain went on scanning ProcessHandle.descendants() for up to a minute - onExit().thenRun { client?.onGradleDaemonExited(pid) } could fire into a client whose RPC channel was being torn down; shutdown() sets client to null, so that was a race rather than a guaranteed no-op - shutdown() was dead code, which made onBuildStarted's note about the scheduler rejecting work after shutdown describe an unreachable state Called before the client is cleared, so no later poll can report into a channel that is going away. Through the lazy delegate rather than the property: touching the property would construct a watcher, and its scheduler, only to shut it down again on a server that never ran a build. Tests: 28 in the module, one new -- shutdown stops the scheduler. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
|
Taken onto #1813 in
if (lazyDaemonWatcher.isInitialized()) {
runCatching { daemonWatcher.shutdown() }
.onFailure { log.warn("Could not stop the Gradle daemon watcher", it) }
}Touching the property would have constructed a watcher, and its scheduler, only to shut it down again on a server that never ran a build. All three of your consequences are addressed, and a test pins that You were right that it pre-dates this branch: it is in |
Finishing the editor while the carousel was undocked unbound and closed the controller that the floating window was still driving: frozen charts, dead camera and CSV buttons, and nothing saying the data source had gone. The watchers stop with MetricsViewModel anyway, so there is no version of this where the floating carousel usefully outlives the editor -- the window has to go too, and now does, before the controller is released. The network watcher's resume gate reads isSupported as well as isWatching. Where TrafficStats has no per-UID counters the loop clears `watching` and breaks, so the gate alone relaunched a coroutine that sampled once, repainted a permanently-zero chart and died -- on every resume, for the life of the session. postProjectInit checks BUILD_CANCELLED before resolving the project name rather than after. The cancel path never used the name, but paid for a workspace-model walk and a catch-Throwable to build it. Not changed, on inspection: review called textScaleFor's lower bound of 1f an undocumented bug, on the grounds that a user on Android's "Small" setting gets chart text coerced up to 1.0. The floor is deliberate and documented -- `a font scale below one does not shrink the chart further` pins it, with the reason: this text is already the smallest on screen, so following a reduction makes it unreadable rather than merely small. I changed it, the test caught it, and it is back. Only the ceiling is the fixed-strip trade-off (ADFA-5634); the KDoc now says which is which. Tests: full app unit suite green, spotlessCheck clean. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
GradleDaemonWatcher.shutdown() had no caller. Reported by hal-eisen-adfa on PR #1812, where the code is in the base commit rather than the diff -- it came in with ADFA-5514 via #1798, so it is live on stage. - the watcher's thread outlived server shutdown, and an in-flight poll chain went on scanning ProcessHandle.descendants() for up to a minute - onExit().thenRun { client?.onGradleDaemonExited(pid) } could fire into a client whose RPC channel was being torn down; shutdown() sets client to null, so that was a race rather than a guaranteed no-op - shutdown() was dead code, which made onBuildStarted's note about the scheduler rejecting work after shutdown describe an unreachable state Two details that are easy to get wrong, both found in review of the version of this fix that rides ADFA-5589: It is called after DefaultGradleConnector.close(), not before. Stopping the daemons is what produces the exit, and the exit is reported through scheduler.execute { ... } -- a scheduler already shut down rejects it and merely logs, so the client never hears that the daemon it is plotting has gone. For the same reason the client is cleared after the wait rather than before it; best effort even then, since the client's own channel is going away at the same time. It goes through the lazy delegate rather than the property, or a server that never ran a build constructs a watcher, and its scheduler, purely to shut it down again. This is carried out of PR #1813, which is a draft while the review queue drains, so the fix does not wait on it. Tests: 9 in GradleDaemonWatcherTest, one new -- shutdown reaches scheduler.shutdownNow(). Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
|
Carried to its own PR against Since it came in with #1798 rather than this branch, and #1813 is a draft while the queue drains, it seemed better not to make the fix wait on either. Both review details are in there — the watcher stops after Thanks for flagging it as pre-existing rather than filing it against this diff; that is what made it obvious it should not ride a feature branch. |
MetricsScratch sized its buffers with maxOf of the three watchers' MAX_USAGE_ENTRIES. That was no guard at all: one size is handed to all three and ShiftedLongArray.copyInto require()s an exact match, so the moment the constants diverge the two smaller watchers throw -- inside MetricsCrashAttachment's runCatching, where it is swallowed and every crash report and feedback send quietly loses its metrics. Exactly the failure the comment claimed to prevent, which is worse than not commenting. sharedRetention() now require()s the three to agree and names them when they do not, so divergence is a loud startup failure instead of a silent hole in diagnostics. samplingJob is assigned only after launch returns. A stop landing in that gap cancels whatever the field held rather than the loop just started, and a later start can overwrite the field with a job nothing then cancels: two loops appending to the same buffers, at twice the sample rate, out of step with the row timestamps. Cancelling more carefully cannot fix it, because the assignments themselves can land out of order, so each loop now carries the generation it was started for and stops as soon as it is not the current one. All three watchers had the shape and all three have the guard. metricsAttachmentForFeedback's KDoc said the snapshot is assembled on the main thread; the body says the opposite two lines below and does the opposite. The KDoc was left over from before the assembly moved off the main thread. viewpager2 is declared through the version-catalog alias that carries a version.ref (1.1.0-beta02) rather than the sibling alias hardcoded to 1.0.0. Two aliases exist for the module; picking the older one left which version wins to conflict resolution across the whole graph. Tests: full app unit suite green, two new for the retention guard -- the disagreement test fails against the maxOf it replaces. The generation guard is not test-pinned: like the CAS in ADFA-5589, the interleaving is not reproducible on demand, and a test that passes either way would say less than this sentence does. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
|
Two findings from the latest review pass are filed rather than fixed here, since both are pre-existing and neither is a patch:
Both are under ADFA-5530. Fixing them here would mean a renderer change with its own test surface, plus a file-format decision, in a PR that is already 110 files. |
Merge hazard: #1812 and #1813 rewrite the same two functions, and one of the clashes is silentI merged these two branches locally twice today — once to retire a review finding about untested merged behaviour, once to build a combined APK — and hit the same traps both times. Recording them here so whoever lands these second doesn't rediscover them. Both PRs rewrite
The conflict git flagsThree hunks in
The clash git does not flag
notifyBuildFailure(result = BuildResult(tasks = ..., buildId = ..., durationMs = ...))
TaskExecutionResult(false, getTaskFailureType(error))It compiles, and the tests pass, because both signatures exist after the merge. It should be: TaskExecutionResult(false, notifyBuildFailure(message.buildId, message.tasks, start, error))Otherwise the failure is classified twice and reported through the older path. Nothing warns; only reading the function catches it. And a warning about resolving it by handMy own resolution left So: after merging these two, read Verified on the merged resultNot on either branch alone — that was the gap. |
…nd dash Undocked, the next arrow and the floating frame's resize grip share the bottom-right corner. The grip is a 28dp touch target, so the two sat close enough to read as one control and to invite a mis-hit on the one gesture that resizes the window. The arrow moves in by the grip's own width. Set from the dockable content rather than the layout because docked there is no grip and no reason to give up the space. Legend entries lose the dash: "IDE 561.89MB", not "IDE - 561.89MB". One format string covers all three pages, so memory, network and power all follow. It has no translations to update. Tests: full app unit suite green. Three label assertions in MemoryUsageChartRendererTest carried the dash and now do not. Not visually confirmed on device: the phone was wiped for the combined build and is back at first-run onboarding, so reaching an undocked carousel means re-provisioning the SDK first. 28dp is the grip's measured touch target, not an eyeballed offset, but "slightly left" is a judgment call and worth a look before it lands. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
On a device whose kernel reports CURRENT_NOW in milliamps -- documented as microamps, and not honoured by every OEM -- the Power series read "n/a" on every sample for the life of the session, while temperature, which has no such filter, plotted normally. MIN_PLAUSIBLE_MICROWATTS was added to catch exactly that misreport, and it does catch it: the product comes out a thousand times small and lands under the floor. It then threw the sample away. Identifying a unit mismatch and answering "no data" is strictly worse than applying the conversion the mismatch implies. Measured on a Galaxy Note 20 Ultra with the editor open after a build: CURRENT_NOW 318 at 3807mV. Taken at face value that is 1,210 microwatts -- 1.2mW for a phone running an IDE -- so it fell under the floor and was dropped. Corrected it is 1.21W, which is what the chart should have been showing all along. A single sample still cannot distinguish a milliamp kernel from a genuinely tiny draw, so this remains a judgement rather than a detector. It is the same judgement the floor already made, now acted on rather than used to drop the reading: the only device drawing single-digit milliwatts is one in deep doze, and a dozing device is not running the build this chart exists to measure. The ceiling still rejects, because that mismatch runs the other way and scaling up would widen it. The correction recomputes from the scaled current rather than scaling the product, which has already been through an integer division and would round to the nearest milliwatt. Tests: full app unit suite green. Three of the seven envelope tests fail against the discard-the-band behaviour, including one built from the device's own reading. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
"Tap to bring them back" now reads "Single-tap to bring them back". The strip's empty area also carries a two-finger gesture, so naming the number of fingers removes the ambiguity. No translations to update. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
The strip lives behind SwipeRevealLayout and is closed on launch, so a
user who never drags the app bar down never sees it -- and paid for it
anyway: three loops reading /proc, TrafficStats and the battery every
tick, three listener chains, and three renderers redrawing a chart
underneath an opaque card.
Measured on a Pixel 6 Pro, editor idle, project loaded, carousel not
revealed, using ADFA-5199's own protocol (/proc/<pid>/stat fields 14+15
over 10s, and two /proc/<pid>/task/*/stat snapshots 8s apart):
before 197 ticks/10s main 77, MemoryUsageWatc 82,
PowerUsageWatch 19, NetworkUsageWat 14
after 132 ticks/10s main 124, no watcher threads at all
ADFA-5199 measured the single-chart version of this at 188 ticks/10s on a
OnePlus and proposed pausing when the widget is not visible. That was
never done, and the carousel tripled the watcher count in the meantime.
Started late rather than paused and resumed: a pause would leave a hole
in the middle of the buffers, and the renderer still positions samples by
index rather than by their recorded time (ADFA-5660). A later start
shortens the history without breaking that assumption.
Undocking is covered too -- the floating window shows the carousel
without the strip ever being dragged open -- and onResume now restarts
only what was already running.
Verified on device: no MemoryUsageWatc, PowerUsageWatch or
NetworkUsageWat thread exists until the strip is revealed, and all three
appear on the first reveal.
Known artifact, and the reason this is worth a look before it lands: the
buffers are zero-filled, so a chart revealed 30s into a session draws a
flat zero line for the part of the window before sampling began. It reads
as "the IDE used no memory", not as "not measured". watchedSinceMillis
already records the truth and the CSV exporter already uses it; the chart
does not. Filed rather than fixed here.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
Everything the metrics carousel is, as one change against current
stage.This supersedes the fourteen stacked PRs listed below. It exists because reviewing them individually had stopped working:
stagemoved under the stack twice today, the bottom four branches had diverged, and three separate defects were found sitting on a later ticket's branch than the one that owned them.What it adds
A metrics carousel in the editor, revealed by a swipe from the top, with three pages:
Plus: build markers on every chart, a sampling-rate chooser on the time axis, long-press help, a PNG snapshot, a CSV export of the whole buffer, the same data attached to crash reports and to feedback, and an undockable floating window.
The sixteen tickets
ADFA-5487, 5486, 5489, 5499, 5510, 5509, 5527, 5515, 5531, 5534, 5526, 5553, 5554, 5542, 5574 — and ADFA-5514, which reached
stageas a squashed commit and is integrated here.What to review hardest
stagesquash-merged ADFA-5514 (#1798), which left git no shared history between its changes and the stack's. Five files conflicted, and the resolutions are behavioural — they compile either way:MemoryUsageWatcher— ADFA-5574 replaced the reflective read withProcessMemoryReader; ADFA-5514 had added a liveness guard to the code that rewrite deleted. Resolution: keep the rewrite, re-apply the guard.The guard is a backstop, not the mechanism. What normally removes a dead daemon is
onGradleDaemonExited→unwatchProcess, which drops the series outright — confirmed on device below. The guard covers the window where the daemon is gone but that report has not arrived, or does not arrive at all. That window is worth covering because of how it fails:smaps_rollupdisappears with the process, soreadKbreportsUNAVAILABLE, latches the process onto the reflective reader, and then repeats the last reading forever —Debug.getMemoryInfoleavesmemInfountouched for a pid that no longer exists. The chart would draw a flat line at the daemon's final size for a process that has gone.EventListenerdaemon callbacks are no longer= Unit.GradleBuildServiceListenerWrapperTestasserts no callback has a default, because a default lets a wrapper inherit silence instead of being asked to forward. 5514 added two, and the wrapper would have swallowed the daemon plot silently. The test was right.GradleBuildService.wrap()— both sides had one, in different places, so git saw two additions.stage's still had the pre-ADFA-5542onBuildFailed(tasks). Dropped; the daemon callbacks were added to the survivor.ToolingApiServerImpl— kept 5514's daemon watcher and ADFA-5542'snotifyBuildFailure, which classifies and notifies in one call.BaseEditorActivity— kept 5514's watch/unwatch methods; they still compile because the watcher moved intoMetricsViewModelbehind a property of the same name.Two test classes now declare
isProcessAlive = { true }: they invent pids, so/prochas nothing for them and the new guard would read every one as gone.Verification
836 unit tests pass across
:appand:subprojects:tooling-api-impl. Measured on a Pixel 6 Pro during the stack's development: the memory read is the cheapest correct one per process, verified againstsmaps_rollupandDebug.getMemoryInfo.Device pass done — Pixel 6 Pro, this branch's APK, covering the resolutions above:
GradleDaemonWatcher: Gradle daemon identified: pid 21942→GradleBuildService: Gradle daemon started→ plottedexitedand the series was removed from the legend, not left flat34°..31°and3W..0WIDE - 561.89MB Gradle Tooling - 65.74MBat 2.0), titles and both arrows fit, the CSV and camera actions stay reachable. Font scale restored to 1.0 afterwards. Caveat, since the bare claim overstates it: chart text is capped atMAX_TEXT_SCALE = 1.5f, so at a 2.0 system scale the legend and axis labels are 25% smaller than the user asked for. Nothing clips partly because nothing grows past 1.5x. The strip is a fixededitor_mem_usage_view_heightwith no scroll, so honouring 2.0 fully needs the strip to grow or scroll -- filed rather than claimed.That exercises resolutions 2 and 3 directly: the daemon callback survives the interface-default removal and reaches the chart through the surviving
wrap().The strip is a fixed
editor_mem_usage_view_heightandMAX_TEXT_SCALEcaps chart text at 1.5x, so growth is bounded by design rather than by scrolling. The one place that is load-bearing is the undocked message, which fills the strip at0dp/0dpwith nowhere to scroll: it needs 140px of the 496px it has at 2x on xhdpi, andMetricsChartLargeTextTestnow measures that instead of assuming it.Still not done: a low-end device check. It is in ADFA-5574's Steps to QA. A Pixel 6 Pro understates the sampling cost, which is the number that work was justified by.
Known, filed, not fixed here
Second device check, on a Galaxy Note 20 Ultra
The verification above was done on a Pixel 6 Pro. A second pass on a Galaxy Note 20 Ultra (SM-N986U, arm64) confirmed the later commits on this branch, and found a bug the Pixel could not have shown.
n/aon every sample before the fix; reads a real wattage after itIDE 561.89MB— the dash between a series name and its value is gone. The quoted legends in the table above (Received - 0 B/s,Power - 20mW) predate that changeWhy only this device.
BATTERY_PROPERTY_CURRENT_NOWis documented in microamps, and this kernel reports milliamps. Measured with the editor open after a build:CURRENT_NOW 318at3807 mVis 1,210 µW taken at face value — 1.2 mW for a phone running an IDE.MIN_PLAUSIBLE_MICROWATTS(10 mW) correctly identified that as a unit mismatch and then discarded the sample, so the series wasUNAVAILABLEfor the life of the session while temperature, which has no such filter, plotted normally. It now applies the conversion the mismatch implies: 1.21 W.The Pixel reports the documented unit, so its readings cleared the floor and the defect was invisible there. That is the argument for the low-end/second-device check still outstanding in ADFA-5574's Steps to QA — a single reference phone hid a permanently broken series on a whole device family.
Confirmed working on device by the author after install.